Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added vertical datum to metadata #165

Open
wants to merge 1 commit into
base: FloodAdapt_ModelBuilders
Choose a base branch
from

Conversation

roeldegoede
Copy link
Collaborator

For datasets, a vertical_datum should be provided in the metadata of the data_catalog.yml file. For newly added datasets, we check whether it is in the files metadata, otherwise the user should specify it.

Once generating the topobathy of the SFINCS model, we add the vertical datum of the first dataset to the models metadata. This can in the end be found in the "sfincs_build.yaml" file under meta, vertical datum, e.g.:

meta:
   vertical_datum: MSL

It isn't used for anything, just for keeping track of the vertical reference level after creating the model. Note, this file is only present for models that are built with the FloodAdapt_ModelBuilders ...

@roeldegoede roeldegoede requested a review from LuukBlom October 11, 2024 12:55
@roeldegoede
Copy link
Collaborator Author

@panosatha @kathrynroscoe

Copy link
Collaborator

@LuukBlom LuukBlom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one comment about validation of the user input

Comment on lines +148 to +151
vdatum, okay = app.gui.window.dialog_string("Provide a vertical datum for the dataset", "MSL")
if not okay:
# Cancel was clicked
return
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow people to enter anything and validate that its an actual vdatum or maybe only select from a list of allowed vdatums?

Not sure about a solution, which ones to allow / how to get them. But allowing any str and not validating might break the app / data catalog when that dataset is used?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roeldegoede is this an issue or should I just merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants